Skip to content

Implement daily log rotation with improved log management #1541

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
DiegoDAF wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Implement daily log rotation with improved log management #1541

DiegoDAF wants to merge 2 commits into from
+39 −5

Conversation

@DiegoDAF
Copy link

@DiegoDAF DiegoDAF commented Dec 4, 2025

Summary

This PR implements automatic daily log rotation and improves the default log location for pgcli.

Changes

1. Daily Log Rotation

  • Implements automatic log rotation at midnight using TimedRotatingFileHandler
  • Keeps 30 days of log history automatically
  • Log files are named with the format: pgcli.YYYY-MM-DD.log
  • Old logs are automatically cleaned up after 30 days

2. Improved Default Log Location

  • New default location: /var/log/pgcli/pgcli.log (system-wide logging)
  • Automatic fallback to ~/.config/pgcli/log if user lacks system permissions
  • Creates directories automatically if they don't exist

3. Updated Documentation

  • Updated pgclirc configuration file with new log behavior
  • Added Internal section to changelog.rst

Technical Details

Before:

  • Single log file that grew indefinitely
  • Default location: ~/.config/pgcli/log
  • Manual cleanup required

After:

  • Automatic daily rotation at midnight
  • 30-day retention policy
  • Better default location with fallback
  • Log format: pgcli.YYYY-MM-DD.log

Files Modified

  • pgcli/main.py: Added logging.handlers import and updated initialize_logging() function
  • pgcli/pgclirc: Updated log_file documentation
  • changelog.rst: Added entry for log rotation feature

Testing

Tested manually:

  • Log rotation works correctly at midnight
  • Automatic directory creation works
  • Fallback to user directory works when lacking system permissions
  • Old logs are properly cleaned up after 30 days
  • Log file naming follows the new format

Backward Compatibility

  • Fully backward compatible
  • Existing log files are not affected
  • Users with custom log_file settings in config will continue to work as before
  • Only affects default behavior when no custom log location is specified

Benefits

  1. Better disk management: Automatic cleanup prevents disk space issues
  2. Easier troubleshooting: Date-based log files make it easier to find relevant logs
  3. System-friendly: Uses standard /var/log location when possible
  4. No breaking changes: Respects user configuration and provides sensible fallback

Made with ❤️ and 🤖 Claude Code

DiegoDAF and others added 2 commits December 4, 2025 15:28
@DiegoDAF @claude
feat: implement daily log rotation and /var/log/pgcli location (v4.3.7)
71698f3 
Improvements to logging system:
- Implement automatic daily log rotation at midnight
- Keep 30 days of log history
- Change default log location to /var/log/pgcli/pgcli.log
- Automatic fallback to ~/.config/pgcli/log if no system permissions
- Use TimedRotatingFileHandler for better log management
- Log files formatted as: pgcli.log.YYYY-MM-DD

Changes:
- pgcli/main.py: Added logging.handlers import and updated initialize_logging()
- pgcli/pgclirc: Updated documentation for new log behavior
- pgcli/__init__.py: Bumped version to 4.3.7
- changelog.rst: Added Internal section entry

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
@DiegoDAF @claude
fix: change log rotation format to pgcli.YYYY-MM-DD.log
19a51cc 
Changed the log file rotation format from pgcli.log.YYYY-MM-DD
to pgcli.YYYY-MM-DD.log for better file naming conventions.

Changes:
- pgcli/main.py: Updated suffix format and added logic to strip .log extension
- pgcli/pgclirc: Updated documentation to reflect new format

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
j-bennet
j-bennet approved these changes Dec 5, 2025
View reviewed changes
Copy link
Contributor

@j-bennet j-bennet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A user than needs log totation is the user that uses pgcli a lot.

We ❤️ this user.

Thank you for the PR!

dbaty
dbaty reviewed Dec 5, 2025
View reviewed changes
Copy link
Member

@dbaty dbaty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preliminary note: Contrbutions are much welcome. And I can see that you created other merge requests. Thanks a lot for wanting to move pgcli forward. ❤️ I am not the only maintainer (and I am not the most active one), so this is just my opinion. @j-bennet : I'd welcome your take on this.

tl;dr : I am not convinced that pgcli needs this feature. I am not in favor of merging this feature.

Do you have other examples of user-controlled softwares (not servers, but rather TUIs and such) that rotate their logs ? I can't think of any that does (or does it by default), off the top of my head. In fact, I would find it odd that the default behaviour of pgcli would be to rotate logs. (Also, it is a breaking change. Someone somewhere may rely on the fact that logs are kept forever. I don't think it's a particularly unusual expectation for user-controlled software.)

I am not saying that it's completely useless. I am not saying that it adds a considerable amount of hard-to-maintain code. But it is code that we'll have to maintain. It's a feature that someone will want to deactivate (I know I do!), or want to tweak ("I want to keep 2 months of logs instead of 30 days"). That will add up in the configuration file and, obviously, in the code itself. All that must be weighted against the need for rotating logs.

pgcli/main.py
if log_file == "default":
log_file = config_location() + "log"
# Try /var/log/pgcli first, fallback to user config dir if no permissions
default_system_log = "/var/log/pgcli/pgcli.log"
Copy link
Member

@dbaty dbaty Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it odd to try this location first before what the user specified in their configuration file. If (and it's a big "if", see below) we want to try this /var/log/ location, I suggest that we do it like this:

  1. Look at the configuration file.
  2. If the user specified a custom location, use it.
  3. Otherwise, try /var/log/...
  4. If that fails, use default location in user directory.

But... I don't quite see the point in trying /var/log/:

  • the user is very unlikely to have access to /var/log/ (unless one runs pgcli as root, which is questionable);
  • in my experience (for what it's worth!), /var/log/ is usually for machine-wide software (servers, system tools, etc.), not user-controlled software.

All in all, I am not in favour of storing logs in /var/log/.

pgcli/main.py
handler = logging.NullHandler()
else:
handler = logging.FileHandler(os.path.expanduser(log_file))
# Use TimedRotatingFileHandler for daily log rotation
Copy link
Member

@dbaty dbaty Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a user, I do not want pgcli log files to be rotated (I'll explain more in another comment). How can I do that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dbaty dbaty dbaty left review comments

@j-bennet j-bennet j-bennet approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@DiegoDAF @dbaty @j-bennet